New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not generate default alt text for images #30213
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
I'm working on fixing up test failures now, but the gist of the PR is the same. |
Something to reiterate from the PR comment: this change will create warnings in some accessibility testing tools. These would be correct failures, but new failures nonetheless. In some cases these warnings could potentially be part of CI suites, and, in an even more narrow subset of applications, may create build failures. Is it worth introducing a configuration option to disable the correct behavior for folks that may need to fix failures when upgrading Rails? The alternative for those that want to delay fixes would be to monkey-patch the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. Nice work. Can you add a CHANGELOG entry?
@@ -286,6 +282,8 @@ def image_tag(source, options = {}) | |||
# image_alt('underscored_file_name.png') | |||
# # => Underscored file name | |||
def image_alt(src) | |||
::ActiveSupport::Deprecation.warn("ActionView::Helpers::AssesTagHelper.image_alt is deprecated and will be removed from Rails 5.2. You must exlicitly set alt text on images.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the first ::
here.
@@ -286,6 +282,8 @@ def image_tag(source, options = {}) | |||
# image_alt('underscored_file_name.png') | |||
# # => Underscored file name | |||
def image_alt(src) | |||
ActiveSupport::Deprecation.warn("ActionView::Helpers::AssesTagHelper.image_alt is deprecated and will be removed from Rails 5.2. You must exlicitly set alt text on images.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/AssesTagHelper/AssetsTagHelper
, s/exlicitly/explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to talk about the module name too. Only image_alt is deprecated
is good. And it is going to be removed from Rails 6.0.
@@ -268,6 +262,8 @@ def image_tag(source, options = {}) | |||
tag("img", options) | |||
end | |||
|
|||
# This method will be deprecated in future versions of Rails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is deprecated now. It’ll be removed in Rails 5.2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's deprecated in 5.2 and will be removed in 6.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though we let the deprecation warning itself suffice, so need for a comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that the comment says this method will eventually be deprecated when it’s actually deprecated in this PR.
The deprecation warning says this method will be removed in 5.2; it should say 6.0.
@@ -286,6 +282,8 @@ def image_tag(source, options = {}) | |||
# image_alt('underscored_file_name.png') | |||
# # => Underscored file name | |||
def image_alt(src) | |||
ActiveSupport::Deprecation.warn("ActionView::Helpers::AssesTagHelper.image_alt is deprecated and will be removed from Rails 5.2. You must exlicitly set alt text on images.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to talk about the module name too. Only image_alt is deprecated
is good. And it is going to be removed from Rails 6.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Can you squash your commits?
75153dc
to
c469879
Compare
unless src.start_with?("cid:") || src.start_with?("data:") || src.blank? | ||
options[:alt] = options.fetch(:alt) { image_alt(src) } | ||
end | ||
options[:src] = path_to_image(source, skip_pipeline: skip_pipeline) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing the method being used to generate src
? Using path_to_image
instead of resolve_image_source
is causing CI to fail 😬
actionview/CHANGELOG.md
Outdated
@@ -1,3 +1,9 @@ | |||
* Remove default `alt` text generation | |||
|
|||
Fixes #30096 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a period after the issue number
Thanks @maclover7, fixed. Good to squash? |
👍 squash again |
- Auto-generating content from the filename of an image is not suitable alternative text; alt text that isn't fully considered can be distracting and fatiguing for screen readers users (blind, low vision, dyslexic people). - Setting a filename fallback short circuits screen reader default behavior and configuration for blank descriptions. - Setting poor defaults also creates false negatives for accessibility linting and testing software, that makes it harder to improve application accessibility. *** - After this change, if authors leave images without alt text, screen readers will fallback to default behavior for missing alt text. - Also with this change, Automated linting and testing tools will correctly generate warnings. [Fixes rails#30096]
c66f5ab
to
9a74c7b
Compare
Squashed and ready! |
@rafaelfranca anything else I can add? |
Do not generate default alt text for images
The `image_tag` helper no longer generates a default alt tag from the filename. The reasoning is explained in [rails PR #30213][0] Since we don't have custom `alt` tags written up for the book images, I just removed the `alt` tags from the spec expectations [0]:rails/rails#30213
Summary
image_alt
helper.image_alt
fromimage_tag
.[Fixes #30096]
Impact
Motivation